-
Notifications
You must be signed in to change notification settings - Fork 264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(ui5-menu): refactor menu items display #8259
Conversation
- This PR affects the ui5-menu and ui5-side-navigation components rendering. - The ui5-menu-item now extends ui5-li-custom class and it will be represented directly as a list item in the DOM. As a result the ui5-menu-item is no longer an abstract class, but a physical component and it will ease the possibility in the future to support custom content supplied by the application developers. - Additionally the ui5-reponsive-popover is no longer rendered into the static area. Thus the ui5-menu structure defined by the application developers is represented more accurately into the DOM and the browser popover API could be used once available for all supported browsers. - There is also a new behavior implemented, which enables the end users to navigate via mouse and keyboard to a disabled ui5-menu-item element. This behavior is aimed to improve the accessibility of the ui5-menu component by providing screen reader announcements for disabled menu items as well. Fixes: SAP#7096
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor
</div> | ||
</ui5-li> | ||
{{/each}} | ||
<slot></slot> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need them as a slot or we need them as individual ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it seems to me now that I've read the documentation that the individual slot scheme would be more suitable for the ui5-navigation-menu
template as there is an anchor tag wrapper around most of the ui5-navigation-menu-item
's:
Sometimes, however, each child must be placed separately in the shadow root, potentially wrapped in other HTML elements, to satisfy the UX design of the component.
The currently implemented slot scheme in the ui5-menu
looks clean. Not sure yet if we could use the general slot approach in the ui5-menu
and at the same time individual slot approach for the ui5-navigation-menu
derived component.
Additional feedback would be greatly appreciated.
const item = e.target as MenuItem; | ||
item.hasSubmenu && await this._prepareSubMenuDesktopTablet(item); | ||
} else if (shouldCloseMenu && this._isSubMenu && this._parentMenuItem) { | ||
const parentItemMenu = this._parentMenuItem.parentElement as Menu; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have that interaction trough events? For example the menu to throw some event to the parent menu like "close-submenu" and then the parent to close it. I don't think that reaching to the parent and calling a method of the parent is good practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is a good idea now that the ui5-menu-item
's do have a DOM representation and they aren't displayed in the static area. My only concern is that it could take additional time to implement and if we'll fit in the time frame. I'll try to evaluate this effort and we could have a follow up discussion about it and make a decision if we'll implement it now or in an additional task.
Hi @unazko, @tsanislavgatev, Please play a bit with following scenario with the proposed implementation: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not merge this PR until the popover API is supported for all browsers.
The
ui5-menu-item
now extendsui5-li-custom
class and itwill be represented directly as a list item in the DOM.
As a result the
ui5-menu-item
is no longer an abstract class,but a physical component and it will ease the possibility
in the future to support custom content supplied by
the application developers.
The
ui5-reponsive-popover
is no longerrendered into the static area. Thus the
ui5-menu
structuredefined by the application developers is represented more
accurately into the DOM and the browser
popover
API couldbe used once available for all supported browsers.
There is now no differentiation between mobile and desktop
device in regards to the display mechanism. In both cases
we rely on the template to do the job as the components used
for composition like
ui5-list
andui5-responsive-popover
docomply with the device.
There is also a new behavior implemented, which enables
the end users to navigate via mouse and keyboard to a
disabled
ui5-menu-item
element. This behavior is aimedto improve the accessibility of the
ui5-menu
componentby providing screen reader announcements for disabled
menu items as well.
The
haspopup
attribute is now applied to theui5-menu item
'swith a sub-menu.
The
ui5-menu
elements used for sub-menus are created only onceand are being reused afterwards. They are no longer destroyed on close.
This contributes to lowering the count of the slow DOM manipulation operations.
The custom focus handling is now done solely in the
ui5-menu
componentas we no longer use the
ui5-responsive-popover
functionalities to retrieve the focus.As a result the code is easier to maintain and trace the focus shifting.
Fixes: #7096
Fixes: #7767
Fixes: #7423
Related to: #6350
Related to: #8214
Related to: #7391